Reset VSCode configuration between tests#1575
Conversation
Instead of using the `glob` library and a custom promise, this will use `glob-promise` which is used by other parts of the codebase already. This reduces the amount of code which manually needs to call `reject` and makes it easier to read.
This class will be used to set test config values for the tests. It is able to set the config value to a specified value for every test and restore the value to the original value after the test.
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
I like this. It seemed a little complicated at first, but after playing around a bit I agree with the approach and I can't see a better way.
I have a few ideas about how to make it more robust and less prone to user error as we add or change settings. Forgive me for writing a bunch of code around this but I felt it was the easiest way to communicate what I meant and to try stuff out to make sure it works before I suggest it. Feel free to dispute anything if you think it's not an improvement or not a good idea.
| } | ||
| } | ||
|
|
||
| export const testConfig = { |
There was a problem hiding this comment.
This is going a bit further than the other suggestions, but I wondered if it would be possible to:
- Avoid specifying a list of setting names here:
- It creates duplication as we have to remember to add any new settings to another location.
- It also means we don't reset all settings unless we manually write all setting names here.
- Avoid tests having to know about and use the test config:
- It makes test code different from production code.
- If some test code calls production code that sets config the normal way, there's a chance this might not get reset. It depends on if that setting is listed here, which it might not be if the test code isn't setting it through the test config.
Instead, could we programatically determine the list of all settings? I think we can. Again I had a go at it just to make sure it was possible. You can see my attempt at d0eca7e. That also reverts some changes from this PR that are no longer necessary.
It's worth noting that contributes.configuration.properties doesn't include every setting, because putting a setting here makes it public. Therefore some settings such as the canary flag and live results flag are deliberately not in this list. You can still do getValue to get that value of such a setting, but you can't do updateValue as then vscode complains the setting isn't defined. This means for these setting we have to use stubs in the tests but this is fine because stubs are restored after each test anyway.
What do you think?
There was a problem hiding this comment.
Update... it doesn't seem to be working and I'm not sure why. Values aren't being reset between tests. I've been using d3d6c41 to test it.
We could have a look into it, either together or separately, if you think it's worth pursuing.
There was a problem hiding this comment.
I did think about automatically adding all settings. I didn't think about using the package.json for it since that doesn't include the canary and live results flags which are the config settings we're now setting most often in tests.
An alternative solution I did think about is keeping track of all created Setting objects and using that to keep a list of all used settings. That would mean the extension has some logic in it purely for tests, but this is fairly minimal (it could be as simple as ALL_SETTINGS.push(this) in the Setting constructor). What do you think about doing it in this way?
There was a problem hiding this comment.
I think adding a line to the Setting constructor that adds it to some list would be ok. I think it would be worth trying it and seeing how well it works in practice.
This will register all settings for which a `Setting` instance is created as settings which will be reset. This should make it less error-prone to change settings in tests.
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
LGTM
Thanks for talking about different options and trying them out.
2 participants
This will introduce a new helper that can be used to set config values in VSCode. Instead of the previous behaviour where it would retain the value after the tests had finished (resulting in e.g. the
codeQL.variantAnalysis.controllerReposetting being changed when you run the CLI tests from VSCode's Run and Debug menu), this will restore the value set by the user after finishing all tests. It will also set the value of specified config settings to a known value before each test.It's probably easiest to review this PR commit-by-commit.
Checklist
ready-for-doc-reviewlabel there.